Skip to content

refactor: extract experimental target loader wrapper#295

Closed
ndycode wants to merge 1 commit intorefactor/pr2-unified-settings-entry-wrapper-3from
refactor/pr2-experimental-target-loader-wrapper-latest
Closed

refactor: extract experimental target loader wrapper#295
ndycode wants to merge 1 commit intorefactor/pr2-unified-settings-entry-wrapper-3from
refactor/pr2-experimental-target-loader-wrapper-latest

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • extract the loadExperimentalSyncTarget wrapper out of settings-hub on top of the latest settings leaf
  • keep retrying file reads and target normalization behavior unchanged while slimming the settings facade
  • add a focused test for the new experimental target loader helper

Validation

  • npm run typecheck
  • npm run lint -- lib/codex-manager/experimental-sync-target-entry.ts lib/codex-manager/settings-hub.ts test/experimental-sync-target-entry.test.ts
  • npm run test -- test/experimental-sync-target-entry.test.ts

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

clean extraction of the loadExperimentalSyncTargetEntry wrapper out of settings-hub.ts — the readJson adapter (wiring readFileWithRetry with windows-safe retry codes into a json-parse helper) is now a standalone injectable unit. behavior is a faithful 1:1 move with no logic changes.

  • experimental-sync-target-entry.ts — new module; assembles readJson from readFileWithRetry + {EBUSY, EPERM, EAGAIN, ENOTEMPTY, EACCES} retry set and maxAttempts: 4, then delegates to loadExperimentalSyncTargetState
  • settings-hub.ts — call-site slimmed to a simple loadExperimentalSyncTargetEntry(...) pass-through; all injected deps (readFileWithRetry, sleep, normalizeAccountStorage) are forwarded correctly
  • experimental-sync-target-entry.test.ts — single delegation test added, but readFileWithRetry is never actually invoked because loadExperimentalSyncTargetState is fully mocked; the windows retry-code assembly is the key extracted behavior and it lacks a covering test

Confidence Score: 4/5

  • safe to merge; one targeted test addition would fully cover the extracted windows retry-code logic
  • the production code change is a pure, behavior-preserving extraction — no logic altered, windows retry codes and maxAttempts carried over exactly. the only gap is that the test doesn't exercise the readJson closure, leaving the retry-code constants unverified; a follow-up test covers it without blocking merge
  • test/experimental-sync-target-entry.test.ts — readJson adapter path is untested

Important Files Changed

Filename Overview
lib/codex-manager/experimental-sync-target-entry.ts new injectable wrapper that assembles readJson from readFileWithRetry + windows retry codes and delegates to loadExperimentalSyncTargetState; behavior is a faithful 1:1 extraction from settings-hub
lib/codex-manager/settings-hub.ts call-site updated to delegate through loadExperimentalSyncTargetEntry; all five retry codes and sleep injection are correctly forwarded, no behavior change
test/experimental-sync-target-entry.test.ts single happy-path test verifies delegation but never exercises the readJson closure, leaving the windows-safe retry code assembly untested

Sequence Diagram

sequenceDiagram
    participant SH as settings-hub.ts
    participant ESTE as experimental-sync-target-entry.ts
    participant EST as experimental-sync-target.ts
    participant RFR as readFileWithRetry

    SH->>ESTE: loadExperimentalSyncTargetEntry({ loadExperimentalSyncTargetState, detectTarget, readFileWithRetry, normalizeAccountStorage, sleep })
    note over ESTE: builds readJson closure<br/>retryableCodes: EBUSY/EPERM/EAGAIN/ENOTEMPTY/EACCES<br/>maxAttempts: 4
    ESTE->>EST: loadExperimentalSyncTargetState({ detectTarget, readJson, normalizeAccountStorage })
    EST->>RFR: readJson(path) → readFileWithRetry(path, retryOptions)
    RFR-->>EST: file contents string
    EST-->>ESTE: { kind, detection, destination? }
    ESTE-->>SH: { kind, detection, destination? }
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/experimental-sync-target-entry.test.ts
Line: 5-27

Comment:
**`readJson` adapter is never exercised**

the test mocks `loadExperimentalSyncTargetState` at the top level, so the `readJson` closure (the *actual* extracted behavior — wiring `readFileWithRetry` with retry codes and `maxAttempts: 4`) is never invoked. `readFileWithRetry` spy call count stays at 0.

if someone changes the retry codes, drops `EACCES`/`EPERM`, or alters `maxAttempts`, this test will still pass. those are the windows filesystem-safety constants this project explicitly guards.

add a second `it` that captures the `readJson` arg passed into `loadExperimentalSyncTargetState` and calls it, then asserts `readFileWithRetry` was called with the expected `retryableCodes` set and `maxAttempts: 4`:

```typescript
it("builds readJson with the correct windows-safe retry codes", async () => {
    const readFileWithRetry = vi.fn(async () => '{"ok":true}');
    let capturedReadJson: ((path: string) => Promise<unknown>) | undefined;

    await loadExperimentalSyncTargetEntry({
        loadExperimentalSyncTargetState: async ({ readJson }) => {
            capturedReadJson = readJson;
            return { kind: "target", detection: { kind: "target" }, destination: null };
        },
        detectTarget: () => ({ kind: "target" }) as never,
        readFileWithRetry,
        normalizeAccountStorage: vi.fn(() => null),
        sleep: vi.fn(async () => undefined),
    });

    const result = await capturedReadJson!("/some/path");
    expect(result).toEqual({ ok: true });
    expect(readFileWithRetry).toHaveBeenCalledWith("/some/path", {
        retryableCodes: new Set(["EBUSY", "EPERM", "EAGAIN", "ENOTEMPTY", "EACCES"]),
        maxAttempts: 4,
        sleep: expect.any(Function),
    });
});
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: extract ex..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f5d53485-ad90-4987-9c86-c3857aa2f2f8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr2-experimental-target-loader-wrapper-latest
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr2-experimental-target-loader-wrapper-latest

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode added the passed label Mar 22, 2026
@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant